Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cxl-ui): add cxl-stats component #291

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

kertuilves
Copy link

@heshfekry
Copy link

This is looking good.

Quick clarification. They only show when they have non-zero values? or?
image

I would change the label from CXL explored to Complete library

@kertuilves kertuilves force-pushed the kertu/feat/dashoard-stats-component branch from 3ef1ce0 to 4ef2acf Compare July 6, 2023 11:16
@kertuilves
Copy link
Author

This is looking good.

Quick clarification. They only show when they have non-zero values? or?

I would change the label from CXL explored to Complete library

@heshfekry The stats count is meant to change the count of items there is in the row. For now there is data for 4 items (max amount in the design) and the count is there to just show what would happen if there is less than 4 items.

@heshfekry
Copy link

heshfekry commented Jul 6, 2023 via email

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Component has max value of 4 items in row which is sufficient for current design. I'm just wondering if it's worth making this component more universal and increase the limit.

@heshfekry what do you think?
@kertuilves what's your estimation on providing such functionality:

  • for max 8 items,
  • for any amount of items?

packages/cxl-lumo-styles/scss/global.scss Show resolved Hide resolved
Comment on lines 17 to 18
<p class="stats-title">${unsafeHTML(el.title)}</p>
<h3 class="stats-count">${el.count}</h3>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is semantically incorrect. Title should use header element and count <p>. It might seem natural to use <h*> because it's bigger by styles, but styles can and should be adjusted.

Also please use <h4> instead of <h3>.

packages/cxl-ui/src/components/cxl-stats.js Outdated Show resolved Hide resolved
packages/cxl-ui/scss/global/cxl-stats.scss Outdated Show resolved Hide resolved
@heshfekry
Copy link

heshfekry commented Jul 7, 2023

Component has max value of 4 items in row which is sufficient for current design. I'm just wondering if it's worth making this component more universal and increase the limit.

@heshfekry what do you think? @kertuilves what's your estimation on providing such functionality:

  • for max 8 items,
  • for any amount of items?

Dont see the need just yet. If its less than 2hrs of work sure.

Otherwise can just duplicate conponent of max 4 to create different stat bundles of max 4 per block.

@kertuilves kertuilves force-pushed the kertu/feat/dashoard-stats-component branch from 4ef2acf to 51fe20d Compare July 10, 2023 11:12
@kertuilves
Copy link
Author

Component has max value of 4 items in row which is sufficient for current design. I'm just wondering if it's worth making this component more universal and increase the limit.
@heshfekry what do you think? @kertuilves what's your estimation on providing such functionality:

  • for max 8 items,
  • for any amount of items?

Dont see the need just yet. If its less than 2hrs of work sure.

Otherwise can just duplicate conponent of max 4 to create different stat bundles of max 4 per block.

I made the changes so when the count is updated and it's bigger than 4, then it just adds last item again and again.

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍 It's almost ready to merge, just minor tweaks needed.

Commit message:

- feat: add dashboard stats component
+ feat(cxl-ui): add `cxl-stats` component

packages/storybook/cxl-ui/cxl-stats/default.stories.js Outdated Show resolved Hide resolved
@kertuilves kertuilves force-pushed the kertu/feat/dashoard-stats-component branch from 51fe20d to 723fe39 Compare July 10, 2023 18:27
@kertuilves kertuilves changed the title feat: add dashboard stats component feat(cxl-ui): add cxl-stats component Jul 10, 2023
@kertuilves kertuilves changed the title feat(cxl-ui): add cxl-stats component feat(cxl-ui): add cxl-stats component Jul 10, 2023
@pawelkmpt pawelkmpt merged commit 8fa92d6 into feat/dashboard Jul 11, 2023
4 checks passed
kertuilves pushed a commit that referenced this pull request Jul 14, 2023
@pawelkmpt pawelkmpt deleted the kertu/feat/dashoard-stats-component branch January 9, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants